Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally remove time crate dependency #137

Closed
wants to merge 4 commits into from

Conversation

jethrogb
Copy link

I would like to use the chrono crate to process time information on systems that don't have a clock. The time crate, which is used by chrono to obtain and represent the local time, can't be made to work on such a system. This PR adds a default-enabled "local" feature to the crate and moves all code that is dependent on the time crate behind a cfg-gate. Current users of chrono need to do nothing. Users of chrono that would like to use it without the time crate can set default-features = false in their Cargo.toml.

Open questions:

  • Test coverage I have fixed all unit tests such that they will pass with or without the local feature. There are however 31 doc tests that use some code that has been cfged-out. I'm not sure how to selectively disable them.
  • UTC UTC::now and UTC::today also depend on the time crate. I have put them behind the same cfg-gate, but that might be a misnomer. We could leave this as is, or I could add another feature with a different name or rename the current feature.

@jethrogb
Copy link
Author

jethrogb commented Mar 9, 2017

Ping. What would it take to get this merged?

@lifthrasiir
Copy link
Contributor

I had been busy finishing and publishing Kailua for recent weeks. Sorry for delay!

I'm pretty okay to make libtime dependency optional, but I don't want to revive time::Duration in this way. The big problem is that we will eventually want to deprecate time::Duration or its copy in favor of something else---this had been std::time::Duration in 0.2 era, but it turned out that its unsignedness made everything less comfortable. I intend to merge this PR after the solution settles down, but not right now.

@@ -4,11 +4,14 @@
//! The UTC (Coordinated Universal Time) time zone.

use std::fmt;
#[cfg(feature="local")]
Copy link
Contributor

@tmccombs tmccombs May 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be #[cfg(not(feature="local"))]?

In which case, UTC::now() needs to be changed to get the current time without using oldtime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is correct. With the local feature enabled, we have extern crate time as oldtime in the crate root. This is needed to get the current time. With UTC::today and UTC::now cfged-out, this import is not used.

@tmccombs
Copy link
Contributor

How about making the time crate dependency optional, and if the time crate isn't provided, define a chrono::Duration struct that allows negative durations (and probably has a similar API to time::Duration)?

It's unfortunate chrono has a dependency on time just for the definition of a single struct.

@jethrogb
Copy link
Author

jethrogb commented May 14, 2017

How about making the time crate dependency optional, and if the time crate isn't provided, define a chrono::Duration struct that allows negative durations (and probably has a similar API to time::Duration)?

That's pretty much exactly what this PR is? @lifthrasiir already said he doesn't want to use that definition.

It's unfortunate chrono has a dependency on time just for the definition of a single struct.

That's not quite true, it also uses the time crate to get the current time and figure out what the local timezone is. See mod local and the things with #[cfg(feature="local")] in mod utc.

@lifthrasiir
Copy link
Contributor

I originally intended to merge this PR in 0.4.0, but it is now slightly delayed to 0.4.1 due to the request to make 0.4.0 available ASAP.

@jethrogb
Copy link
Author

If I rebase this, will you merge it?

@CryZe
Copy link
Contributor

CryZe commented Jan 27, 2018

Can we get this merged in some way? The wasm-unknown-unknown target can't really use chrono otherwise.

@udoprog
Copy link

udoprog commented Feb 19, 2018

I'd like to put a big 👍 on getting this fixed. I outlined the issues I had with chrono here when porting one of my projects to wasm, and I just wanted to parse ISO 8601 timestamps:
https://udoprog.github.io/rust/2018-02-19/porting-rust-to-wasm.html#whats-the-time-again

@davidhewitt
Copy link

+1 would also love to see this! I'm experimenting with isomorphic Rust (server + wasm) and would very much like to use chrono types to represent timestamps in the data passing between server and client but unfortunately this time dependency is getting in the way!

Let me know if I can do anything to help 😄

@CryZe
Copy link
Contributor

CryZe commented Mar 13, 2018

@jethrogb We can potentially rebase this for you so this moves ahead faster if you want. There's a lot of interest in seeing this merged now.

@jethrogb
Copy link
Author

@CryZe I can rebase it myself no problem. I'm just waiting for a statement from the maintainer that if and when I do, it will get merged.

@quodlibetor
Copy link
Contributor

Sorry, yes, @lifthrasiir has expressed string opposition to merging this, and this is their project.

@jethrogb
Copy link
Author

jethrogb commented Mar 14, 2018

@quodlibetor can you elaborate on the "strong opposition"? Where was this expressed? Last statement from @lifthrasiir was “I originally intended to merge this PR ...”. I'm happy to make changes as requested, but there is no actual proposal on how to change this PR to suit the maintainer's wishes.

Note this PR is pretty much a prerequisite for no_std compatibility discussed in #195.

@quodlibetor
Copy link
Contributor

My understanding (from this comment and this comment in another issue)was that @lifthrasiir wants to replace Duration with chrono::TimeSpan. I haven't seen any work in this space, but the opposition that I thought I saw was to Duration at all, not to getting rid of the time crate dependency.

@quodlibetor
Copy link
Contributor

@jethrogb I've been made a release manager for the chrono org, if you rebase this I'll re-review it with the intention of merging it.

My biggest request at this point would be for you to enhance the commit messages a bit:

  • Why do you remove span
  • Could you mention where you're importing (and I slightly prefer the termvendoring for this) oldtime from (just a web link or something)

Other than that I think that the fact that this is all behind feature flags means that we can let the good beat out the perfect for at least a little while longer.

@quodlibetor
Copy link
Contributor

Also, just to be clear, I'd love to get clippy working in no-std as full-featured as possible. I'm not at all invested in the embedded space so I'm not sure what that would look like, but almost the only allocations we require are parsing format strings and outputting the results of format strings, so given a Write or some other sink we could probably get rid of them. behind a std feature.

@jethrogb
Copy link
Author

Rebased and made requested changes. Open questions from original PR remain.

@quodlibetor
Copy link
Contributor

@jethrogb

  • regarding the feature name, should we rename it to just "clock", since what we're doing is using the local clock?
  • It's a little embarrassing, but I'm unclear on exactly what you mean by the doctests that are using features that are cfg'd out? Aren't those doctests also cfg'd out when the feature is disabled? Or is something that I'm missing still a problem?

@quodlibetor quodlibetor self-assigned this Apr 1, 2018
@jethrogb
Copy link
Author

jethrogb commented Apr 2, 2018

Agreed on naming the feature clock.

It's a little embarrassing, but I'm unclear on exactly what you mean by the doctests that are using features that are cfg'd out? Aren't those doctests also cfg'd out when the feature is disabled? Or is something that I'm missing still a problem?

The doctests rely on code that doesn't exist if you don't use the local feature, so they don't compile. I don't know how to apply a cfg to a doctest. This is not a problem for CI, as the doctests are not run for every feature combination in CI.

@jethrogb
Copy link
Author

jethrogb commented Apr 2, 2018

Updated feature name

@quodlibetor
Copy link
Contributor

Ah, I see we're not running with cargo test --no-default-features at all. I agree that that is unfortunate. There's no way to just not run the doctests either, afaict. I would strongly prefer to test with --no-default-features, I'll do some more research.

cc #236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants